Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-48480] Switch deprecated protocols to opt-in. #3188

Merged
merged 6 commits into from
Jun 17, 2018

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Dec 11, 2017

They no longer need to be removed by setup wizard since they won't be
enabled in the first place.

See JENKINS-48480.

Proposed changelog entries

  • Deprecated agent protocols have been switched to opt-in. If you are upgrading directly from old versions of Jenkins (<2.16) and require these protocols, you will need to manually enable them in "Manage Jenkins > Configure Security > Agent Protocols"

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees

They no longer need to be removed by setup wizard since they won't be
enabled in the first place.
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the effect on upgrades would be from this change.

}

@Override
public String getName() {
// we only want to force the protocol off for users that have explicitly banned it via system property
// everyone on the A/B test will just have the opt-in flag toggled
// TODO strip all this out and hardcode OptIn==TRUE once JENKINS-36871 is merged
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2492 FTR

@@ -61,6 +67,14 @@

private ExecutorService pool;

@Before
public void setUp() {
jenkins.CLI.get().setEnabled(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary; it is still enabled by default in core, disabled in wizard.

@jglick jglick requested a review from oleg-nenashev December 11, 2017 17:31
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will break the upgrading instances with old Remoting versions

@Vlatombe
Copy link
Member Author

@oleg-nenashev Ah, I see what you mean. So to fix this means any protocol that was previously opt-out and switched to opt-in, should actually be enabled on upgrade, unless it was explicitly disabled before.

@oleg-nenashev
Copy link
Member

@Vlatombe even this is not going to help in general. E.g. configuration-as-code instances with disabled setup wizard will start behaving differently. So such change should be done with a preliminary announcement (maybe as a part of Jenkins 3)

@daniel-beck
Copy link
Member

change should be done with a preliminary announcement

We have a changelog, should be good enough for something minor like this.

@jglick
Copy link
Member

jglick commented Dec 11, 2017

Which kinds of upgraded instances would be affected? IIUC this would mean that some users of rather old Jenkins versions, predating the config UI that lets you explicitly select protocol versions, upon updating directly to this change would have the old and insecure agent protocols disabled after the upgrade, so if they also had obsolete slave.jars, those would fail to connect until they either updating the agent or reënabled the old protocol (dismissing the warnings about the risks of doing so). Which seems acceptable to me. Or did I mix this up?

@daniel-beck
Copy link
Member

Or did I mix this up?

That is also my understanding.

Which seems acceptable to me.

I agree. This is fairly minor, easy to recover from, and the changelog/upgrade guide can cover this.

@oleg-nenashev
Copy link
Member

I propose to do it after https://issues.jenkins-ci.org/browse/JENKINS-48766 is implemented and released. It would add extra logging and UI info so the update will be more smooth to users

@oleg-nenashev oleg-nenashev added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Jan 6, 2018
@daniel-beck
Copy link
Member

@oleg-nenashev It is unclear to me how these are related.

What's the ETA for that issue?

@ghost
Copy link

ghost commented Jan 11, 2018

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev
Copy link
Member

@daniel-beck ETA depends on my employer, cannot commit on it. Regarding the relation between issues, I want Remoting older than 3.0to be deprecated in LTS before we rollout this breaking change.

@daniel-beck
Copy link
Member

@oleg-nenashev

Regarding the relation between issues, I want Remoting older than 3.0to be deprecated in LTS before we rollout this breaking change.

Remoting 3.0 went into 2.27, 1.5 years ago. How much longer do you want 2.x block obvious improvements to Jenkins?

We should just add changelog notes and stop worrying about every tiny thing. People will read the changelog once their Jenkins breaks, and it'll tell them to update their agents. Problem solved.

@oleg-nenashev
Copy link
Member

@daniel-beck I am working a lot on support fronts in my company, and I see Remoting 2.x in production almost every second day. So it's not a tiny thing. As a Remoting maintainer, I want to handle it properly.

Once I get some promised Remoting maintenance time, I will do that. The question is only whether it lands in the next LTS or in the next next.

@oleg-nenashev
Copy link
Member

In such case we will be able to merge Vincent's PR in ~2 weeks even if I do not deliver on the Remoting Minimal versions. Does it sound like a deal?

@oleg-nenashev oleg-nenashev removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Mar 10, 2018
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR I have delivered on the minimal version check API in https://issues.jenkins-ci.org/browse/JENKINS-48766 (Jenkins 2.104+). There is a follow-up for the agent side (JENKINS-50095), but IMHO it is not a blocker.

@Vlatombe please bump the remoting.minimum.supported.version in the root pom.xml to 3.4+ or so. It should be enough to unlobk this story taking the proper announcement.

@oleg-nenashev oleg-nenashev added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Mar 10, 2018
@Vlatombe Vlatombe removed the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Jun 1, 2018
@Vlatombe
Copy link
Member Author

Vlatombe commented Jun 1, 2018

@oleg-nenashev I added your required change.

@oleg-nenashev oleg-nenashev dismissed their stale review June 1, 2018 13:17

dismissed

@oleg-nenashev
Copy link
Member

It will be still a major breaking change for Jenkins users, who run old protocol versions. So I anticipate a bunch of tickets in JIRA even if there is a warning notice in changelog/upgrade guidelines. My comments are addressed, and I do not block the merge.

If @daniel-beck @dwnusbaum @jeffret-b and other @jenkinsci/code-reviewers are fine with that, please feel free to go forward.

@jeffret-b
Copy link
Contributor

Let me see if I understand.

This change will break the distributed system configuration for some number of Jenkins users. If they know how to fix it, they can do so in a fairly simple fashion. There is an argument that they shouldn't be using these insecure protocols, but their security concerns may be minimal in their environment. There is another argument that these protocols have been deprecated for a long time and we should no longer be concerned with assisting those who won't upgrade.

My biggest concern with the proposed change is that the impact of this change isn't sufficiently communicated. Isn't the proposed changelog entry kind of a side-effect of the bigger change? It doesn't provide users who might be impacted with sufficient warning or troubleshooting information.

A smaller concern is that affected users would have to make the same corrections every time they upgrade. A one-time correction on upgrade is one thing, but having to do it each time is a greater burden and dissuades people from upgrading. However, given the long deprecation of these protocols that may not be a serious concern.

Not long ago there was some heated discussion on one of the Jenkins email lists about how frequently Jenkins upgrades break things. Without better communication, I think this change may hit that stereotype. Or is there better communication on this somewhere that I'm missing?

@daniel-beck
Copy link
Member

SSH Slaves always have latest remoting. Windows agents set up in the last X months/years should have auto-update enabled (I don't know whether it's retroactive, but doubt it).

What's else? Manual slave.jar downloads?

I still think banner for the weeklies and upgrade guide for LTS should do this. It's less of a breaking change (like JEP-200 or some of our worst security fixes were), just a nontrivial upgrade for some.

Also, I bet this 'forced upgrade' will reduce the number of people complaining about remoting being terrible, just because they're on ancient releases lacking tons of robustness and diagnosability improvements 😸

@Vlatombe
Copy link
Member Author

Vlatombe commented Jun 4, 2018

A smaller concern is that affected users would have to make the same corrections every time they upgrade. A one-time correction on upgrade is one thing, but having to do it each time is a greater burden and dissuades people from upgrading. However, given the long deprecation of these protocols that may not be a serious concern.

I don't think this is a concern. This would only affect people who upgrade from a version prior introduction to Agent protocol selection (<2.16) or who never saved Jenkins global configuration since it got introduced. For those people, re-enabling the deprecated protocols (if they really want it) would be a one-time thing and they would never be bothered again.

@oleg-nenashev
Copy link
Member

NOTE: I do not commit to provide any kind of issue triage in JIRA if it gets merged in such way. All reports about not working agents will have to be triaged by somebody else. Does anybody commit to do so?

@jeffret-b
Copy link
Contributor

It sounds like I misunderstood regarding a need to repeat the manual re-configuration step so I withdraw that concern.

My remaining concern is that we communicate about this change. Specifically, I think more is occurring with this change than is indicated in the proposed changelog entry.

@oleg-nenashev
Copy link
Member

We can always send a heads-up blogpost before the release to increase the visibility a bit

@dwnusbaum
Copy link
Member

As long as we add a changelog entry, section to the upgrade guide, and/or blog post noting something along the lines of the following then I am fine with the change:

Deprecated agent protocols have been switched to opt-in. If you are upgrading directly from old versions of Jenkins (can we specify an exact version range?) and require these protocols, you will need to manually enable them in "Manage Jenkins > Configure Security > Agent Protocols".

@jeffret-b
Copy link
Contributor

I agree with Devin. At a minimum it needs to go into the changelog. It would be better if it could go into one or two of those other places.

@daniel-beck
Copy link
Member

Looks reasonable. The minimized configuration approach for opt in/opt out is a bit annoying here, as everyone will just get these disabled, but it is what we have.

It's little different from semi-notable changes we've done in the past. This isn't "Java 8 or Jenkins doesn't start" levels of serious, but rather similar to "Build no longer implies Cancel" or like a number of "major rfe" changes (big blue circles in the changelog). So appropriate regular changelog and LTS upgrade guide entries should do it.

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 9, 2018
@jeffret-b
Copy link
Contributor

A question on process with this PR: Does the proposed changelog entry at the top need to be changed to something more like what Devin suggested? Or, is that something that will be changed / added when the changelog gets assembled? (Or forgotten?)

@Vlatombe
Copy link
Member Author

Updated the changelog entry. FTR, the agent protocol selection has been added in 2.16 (https://issues.jenkins-ci.org/browse/JENKINS-37032).

@jeffret-b jeffret-b self-requested a review June 15, 2018 16:57
@oleg-nenashev
Copy link
Member

I am fine with shipping in the next weekly

@oleg-nenashev
Copy link
Member

Taking the feedback above, I am going to merge the pull request so that we get more soak testing in the weekly release before it goes to LTS.

@oleg-nenashev oleg-nenashev merged commit a372aff into jenkinsci:master Jun 17, 2018
@Vlatombe Vlatombe deleted the JENKINS-48480 branch June 18, 2018 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants